refactor(utils): canonical subscriber-set observable plus exact-dupe cleanups by kojiwakayama · Pull Request #3047 · veryfront/veryfront-code

kojiwakayama · 2026-07-23T17:14:55Z

Summary

Third PR in the code-reduction campaign (after #3045, #3046). Six modules hand-rolled the same Set-based subscribe/notify observable with inconsistent notify semantics; this adds one canonical createSubscriberSet to #veryfront/utils and migrates the accidental copies, plus two byte-identical dupe groups flagged by deno task dupes, plus fixes for two review findings against the already-merged #3045.

Note (force-pushed): the original push accidentally contained only the 2 new files — a git stash/stash pop during verification unstaged the 9 migrated files before commit. The amended commit contains the full change set; the earlier bot comments about dead/unwired code are resolved.

Net: −1 line (+163/−164, 13 files). The raw counter is thin because ~40 lines are review-driven additions fixing #3045 fallout (see below); the consolidation itself removes ~120 lines of duplicated logic. 2634 unit tests pass.

Changes

createSubscriberSet<Args>(onListenerError?) — subscribe→unsubscribe, snapshot-during-notify, isolated listener errors (including a throwing onListenerError itself), size/clear. Migrated: observability/error-collector, observability/log-buffer, server/reload-notifier (both listener sets, per-set error logs preserved; dead getInvalidateListenerCount removed), workflow/claude-code/websocket-publisher.
Two of these sites previously iterated the live Set during notify — a listener unsubscribing mid-notify could skip listeners. Snapshot semantics closes that bug class.
Server-Timing dedupe — the metric-building loop plus roundMs/formatDuration/sanitizeMetricName were duplicated between proxy/server-timing.ts and observability/request-profiler.ts; proxy now uses the exported buildServerTimingValue/roundMs and the format helpers went back to private.
modules/server — four identical findFirstSecureFile/findFirstPlatformFile copies collapsed into one structurally-typed findFirstExistingFile (fs-probe.ts).
refactor(utils): consolidate hash, sleep, and base64 helpers onto canonical utils #3045 review fixes (Codex/Copilot findings on the merged PR):
- UTF-8 regression in plugin-loader: encodeBase64(code) for data: URLs emits Latin-1 bytes for chars in [0x80–0xFF] via btoa, corrupting non-ASCII plugin source outside Deno. Now encodeBase64Bytes(new TextEncoder().encode(code)), restoring the pre-refactor(utils): consolidate hash, sleep, and base64 helpers onto canonical utils #3045 always-UTF-8 behavior.
- Bare btoa in base64urlEncodeBytes: now routed through the new fallback-safe encodeBase64Bytes (btoa → Buffer → clear error), and encodeBase64's non-Latin1 fallback reuses it.

Deliberately left (documented in code or genuinely different)

rendering/client/navigation-store + react/runtime/core mirror — documented cross-bundle contract; react runtime bundle intentionally imports nothing
workflow/claude-code/event-publisher — throwing handlers intentionally propagate; converting would change behavior
proxy Redis channel map — keyed pubsub with connection lifecycle, different abstraction

Verification

deno task verify:quick exit 0; deno task test:unit 2634 passed / 0 failed; affected suites (observability, proxy, modules/server, workflow, utils, html) pass unmodified; pre-push hook (fmt + full suite) passed on the amended commit

Copilot

Pull request overview

Adds new shared utilities intended to reduce duplicated patterns across the codebase, specifically a canonical subscribe/notify listener registry and a shared filesystem probe helper.

Changes:

Introduces createSubscriberSet() in src/utils/subscriber-set.ts to standardize Set-backed subscribe/notify behavior (snapshot during notify, optional listener-error hook).
Introduces findFirstExistingFile() in src/modules/server/fs-probe.ts to consolidate repeated “probe a list of paths via stat” logic.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/utils/subscriber-set.ts	Adds a generic subscriber-set helper intended to replace duplicated Set-based observables.
src/modules/server/fs-probe.ts	Adds a helper to probe multiple candidate paths and return the first existing file.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eada5dd87d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

…cleanups Six modules hand-rolled the same Set-based subscribe/notify observable with inconsistent notify semantics (live vs snapshot iteration, varying error isolation). This adds createSubscriberSet to #veryfront/utils and migrates the accidental copies, plus two byte-identical dupe groups found by the duplicate scanner: - createSubscriberSet<Args>(onListenerError?): snapshot-during-notify (safe mid-notify unsubscribe) with isolated listener errors; migrated observability/error-collector, observability/log-buffer, server/reload-notifier (both listener sets, log messages preserved), and workflow/claude-code/websocket-publisher - Server-Timing helpers (roundMs, formatDuration, sanitizeMetricName) deduplicated: proxy/server-timing now imports them from observability/request-profiler - modules/server: four identical findFirstSecureFile/findFirstPlatformFile copies collapsed into one structurally-typed findFirstExistingFile Deliberately left: rendering/client navigation-store + react/runtime mirror (documented cross-bundle contract), workflow event-publisher (throwing handlers intentionally propagate), proxy Redis channel map (keyed lifecycle, different shape).

kojiwakayama · 2026-07-23T17:35:39Z

Review comments addressed in the force-pushed commit:

Missing call sites / not exported (Copilot, Codex) — correct catch, thank you. The original commit accidentally contained only the 2 new files (a git stash pop during verification unstaged the 9 migrated files before commit; the pre-push test suite passed because the working tree was complete). The amended commit contains the full migration: barrel export, error-collector, log-buffer, reload-notifier, websocket-publisher, proxy server-timing, and both modules/server call-site files.
Throwing onListenerError escapes isolation (Copilot) — fixed; the handler is now wrapped so it can't break notification.
fs-probe docstring ambiguity (Copilot) — clarified: first path in paths order wins, probes run in parallel.

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/utils/subscriber-set.ts:23

createSubscriberSet is a new canonical primitive that changes notification semantics across multiple modules (snapshot iteration, error isolation). It should have focused unit tests covering: (1) snapshot behavior when listeners unsubscribe/subscribe during notify, (2) listener exceptions not stopping later listeners, and (3) a throwing onListenerError not breaking notification.

export function createSubscriberSet<Args extends unknown[] = []>(
  onListenerError?: (error: unknown) => void,
): SubscriberSet<Args> {
  const listeners = new Set<(...args: Args) => void>();

…r test off deleted accessor

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

…ASCII punctuation

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

… names

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

…shot semantics

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings July 23, 2026 17:14

kojiwakayama requested a review from kwakayama as a code owner July 23, 2026 17:14

Copilot started reviewing on behalf of kojiwakayama July 23, 2026 17:15 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/utils/subscriber-set.ts

Comment thread src/utils/subscriber-set.ts

Comment thread src/modules/server/fs-probe.ts Outdated

Comment thread src/modules/server/fs-probe.ts Outdated

chatgpt-codex-connector Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread src/utils/subscriber-set.ts

Comment thread src/modules/server/fs-probe.ts

Copilot AI review requested due to automatic review settings July 23, 2026 17:35

kojiwakayama force-pushed the refactor/consolidate-subscriber-sets branch from eada5dd to 121b590 Compare July 23, 2026 17:35

Copilot started reviewing on behalf of kojiwakayama July 23, 2026 17:35 View session

kojiwakayama mentioned this pull request Jul 23, 2026

refactor(utils): consolidate hash, sleep, and base64 helpers onto canonical utils #3045

Merged

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/utils/base64url.ts

fix(utils): prefer Buffer in encodeBase64Bytes; update reload-notifie…

4eb5be1

…r test off deleted accessor

Copilot AI review requested due to automatic review settings July 23, 2026 17:47

Copilot started reviewing on behalf of kojiwakayama July 23, 2026 17:48 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/utils/base64url.ts

docs(utils): state encodeBase64's Latin-1-vs-UTF-8 semantics precisely

1298efe

Copilot AI review requested due to automatic review settings July 23, 2026 18:59

Copilot started reviewing on behalf of kojiwakayama July 23, 2026 18:59 View session

kwakayama previously approved these changes Jul 23, 2026

View reviewed changes

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/utils/subscriber-set.ts Outdated

Comment thread src/utils/subscriber-set.ts

test(utils): cover subscriber-set snapshot and isolation guarantees; …

9a907a1

…ASCII punctuation

Copilot AI review requested due to automatic review settings July 23, 2026 19:34

kojiwakayama dismissed kwakayama’s stale review via 9a907a1 July 23, 2026 19:34

Copilot started reviewing on behalf of kojiwakayama July 23, 2026 19:34 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/observability/request-profiler.ts Outdated

fix(observability): sanitize the Server-Timing total label like phase…

5113a9f

… names

Copilot AI review requested due to automatic review settings July 24, 2026 05:44

Copilot started reviewing on behalf of kojiwakayama July 24, 2026 05:44 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/utils/subscriber-set.test.ts Outdated

test(utils): make the mid-notify unsubscribe test actually prove snap…

0d6fe72

…shot semantics

Copilot AI review requested due to automatic review settings July 24, 2026 05:54

Copilot started reviewing on behalf of kojiwakayama July 24, 2026 05:54 View session

Merge branch 'main' into refactor/consolidate-subscriber-sets

c3c94af

kwakayama enabled auto-merge (squash) July 24, 2026 05:55

kwakayama approved these changes Jul 24, 2026

View reviewed changes

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 24, 2026 05:57

Copilot started reviewing on behalf of kojiwakayama July 24, 2026 05:58 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Merge branch 'main' into refactor/consolidate-subscriber-sets

da4bbac

Copilot AI review requested due to automatic review settings July 24, 2026 06:07

Copilot AI reviewed Jul 24, 2026

kwakayama approved these changes Jul 24, 2026

View reviewed changes

kwakayama merged commit 119ca7e into main Jul 24, 2026
28 checks passed

kwakayama deleted the refactor/consolidate-subscriber-sets branch July 24, 2026 06:13

Uh oh!

Conversation

kojiwakayama commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Deliberately left (documented in code or genuinely different)

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

kojiwakayama commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kojiwakayama commented Jul 23, 2026 •

edited

Loading